Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[BugFix] Fix frontend multiprocessing hang #7217

Merged
merged 10 commits into from
Aug 7, 2024
Merged

[BugFix] Fix frontend multiprocessing hang #7217

merged 10 commits into from
Aug 7, 2024

Conversation

maxdebayser
Copy link
Contributor

@maxdebayser maxdebayser commented Aug 6, 2024 •
edited
Loading

Now that the OpenAI server has optional multiprocessing, there is a hang that can happen if the backend dies during initialization and never replies to the IS_SERVER_READY message sent in async_engine_client.setup() .

This PR add an optional timeout to _send_one_way_rpc_request so that during the initialization we can check periodically if the server process is still running while we wait for the reply to the IS_SERVER_READY message.

FIX #7213

FYI @njhill @robertgshaw2-neuralmagic

@maxdebayser
Fix frontend multiprocessing hang
c7ea323 
If the server dies, the frontend keeps waiting for it to come up for
ever

Signed-off-by: Max de Bayser <mbayser@br.ibm.com>
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 6, 2024

👋 Hi! Thank you for contributing to the vLLM project.
Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which consists a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of default ones by unblocking the steps in your fast-check build on Buildkite UI.

Once the PR is approved and ready to go, please make sure to run full CI as it is required to merge (or just use auto-merge).

To run full CI, you can do one of these:

  • Comment /ready on the PR
  • Add ready label to the PR
  • Enable auto-merge.

🚀

@maxdebayser maxdebayser mentioned this pull request Aug 6, 2024
Merged
njhill
njhill reviewed Aug 6, 2024
View reviewed changes
Copy link
Member

@njhill njhill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @maxdebayser

vllm/entrypoints/openai/rpc/client.py Outdated Show resolved Hide resolved
vllm/entrypoints/openai/rpc/client.py Outdated Show resolved Hide resolved
vllm/entrypoints/openai/api_server.py Outdated Show resolved Hide resolved
@njhill njhill changed the title Fix frontend multiprocessing hang [BugFix] Fix frontend multiprocessing hang Aug 6, 2024
@robertgshaw2-neuralmagic
Copy link
Collaborator

I wonder if there is a way we can send a health check rather than waiting on timeout. 1000 seconds is 15minutes, this is a very long time to wait before detecting failure. But 1000 seconds is also not that long if we are downloading L405

@maxdebayser
Copy link
Contributor Author

I wonder if there is a way we can send a health check rather than waiting on timeout. 1000 seconds is 15minutes, this is a very long time to wait before detecting failure. But 1000 seconds is also not that long if we are downloading L405

Actually the unit is milliseconds

@robertgshaw2-neuralmagic
Copy link
Collaborator

robertgshaw2-neuralmagic commented Aug 7, 2024 •
edited
Loading

I wonder if there is a way we can send a health check rather than waiting on timeout. 1000 seconds is 15minutes, this is a very long time to wait before detecting failure. But 1000 seconds is also not that long if we are downloading L405

Actually the unit is milliseconds

I see - I think this could be a problem b/c download times can be much longer than the timeout here. Im going to post an alternate proposal

^Ignore I see the polling loop now

@njhill
Copy link
Member

njhill commented Aug 7, 2024

yeah this effectively just polling once per second, maybe add a comment since that confused me too initially… I think some of this can be refactored a bit soon anyhow

@robertgshaw2-neuralmagic
Copy link
Collaborator

Okay, I see now where the polling is

@robertgshaw2-neuralmagic
Copy link
Collaborator

robertgshaw2-neuralmagic commented Aug 7, 2024 •
edited
Loading

Alternatively, we could go with the following:

@maxdebayser
address review comments
c120770 
Signed-off-by: Max de Bayser <mbayser@br.ibm.com>
@maxdebayser
Merge branch 'fix_multiprocess_hang' of github.com:maxdebayser/vllm i…
9aea142 
…nto fix_multiprocess_hang
@robertgshaw2-neuralmagic
Copy link
Collaborator

I think your approach is better than mine. Can you please add a test case for this?

@maxdebayser
improve exception message
33e503b 
Signed-off-by: Max de Bayser <mbayser@br.ibm.com>
@maxdebayser
add unit to constant name
fbefbe0 
Signed-off-by: Max de Bayser <mbayser@br.ibm.com>
@maxdebayser
Expand comment about zmq socket options
72c4fb7 
Signed-off-by: Max de Bayser <mbayser@br.ibm.com>
@maxdebayser
add unit test
6283c66 
Signed-off-by: Max de Bayser <mbayser@br.ibm.com>
@maxdebayser
Copy link
Contributor Author

I think your approach is better than mine. Can you please add a test case for this?

Thanks! I've added a new file for the test, I wasn't sure if there's an existing test file that is a good fit for this.

@robertgshaw2-neuralmagic
Copy link
Collaborator

Ill wait for nick's final signoff, but LGTM. Thanks for the fix!

njhill
njhill approved these changes Aug 7, 2024
View reviewed changes
Copy link
Member

@njhill njhill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @maxdebayser

@njhill njhill added the ready ONLY add when PR is ready to merge/full CI is needed label Aug 7, 2024
@robertgshaw2-neuralmagic robertgshaw2-neuralmagic merged commit fde47d3 into vllm-project:main Aug 7, 2024
51 checks passed
sfc-gh-mkeralapura pushed a commit to sfc-gh-mkeralapura/vllm that referenced this pull request Aug 12, 2024
@maxdebayser @robertgshaw2-neuralmagic @sfc-gh-mkeralapura
[BugFix] Fix frontend multiprocessing hang (vllm-project#7217)
427062b 
Signed-off-by: Max de Bayser <mbayser@br.ibm.com>
Co-authored-by: Robert Shaw <114415538+robertgshaw2-neuralmagic@users.noreply.github.com>
kylesayrs pushed a commit to neuralmagic/vllm that referenced this pull request Aug 17, 2024
@maxdebayser @robertgshaw2-neuralmagic @kylesayrs
[BugFix] Fix frontend multiprocessing hang (vllm-project#7217)
df8378a 
Signed-off-by: Max de Bayser <mbayser@br.ibm.com>
Co-authored-by: Robert Shaw <114415538+robertgshaw2-neuralmagic@users.noreply.github.com>
fialhocoelho pushed a commit to opendatahub-io/vllm that referenced this pull request Aug 22, 2024
@maxdebayser @robertgshaw2-neuralmagic @fialhocoelho
[BugFix] Fix frontend multiprocessing hang (vllm-project#7217)
80e0d14 
Signed-off-by: Max de Bayser <mbayser@br.ibm.com>
Co-authored-by: Robert Shaw <114415538+robertgshaw2-neuralmagic@users.noreply.github.com>
@maxdebayser maxdebayser deleted the fix_multiprocess_hang branch August 27, 2024 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@njhill njhill njhill approved these changes

@robertgshaw2-neuralmagic robertgshaw2-neuralmagic robertgshaw2-neuralmagic approved these changes

Assignees
No one assigned
Labels
ready ONLY add when PR is ready to merge/full CI is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: With frontend multiprocessing the openai server hangs if there is an initialization error
3 participants
@maxdebayser @robertgshaw2-neuralmagic @njhill